Skip to content

#75 crud for patients and users#76

Merged
LinusWestling merged 9 commits into
mainfrom
#75-crud-for-patients-and-users
Apr 22, 2026
Merged

#75 crud for patients and users#76
LinusWestling merged 9 commits into
mainfrom
#75-crud-for-patients-and-users

Conversation

@Tyreviel

@Tyreviel Tyreviel commented Apr 22, 2026

Copy link
Copy Markdown
Collaborator

closes #75

Summary by CodeRabbit

  • New Features

    • Edit and delete flows for employees and patients in the UI (forms, actions, confirmations)
    • REST endpoints to update and delete employees and patients
    • Manager-only access for create/update/delete operations
  • Validation / Behavior

    • GitHub username is immutable (attempts to change are rejected)
    • Personal identity number validated (format enforced) and uniqueness enforced
  • Tests

    • Added comprehensive tests covering create, update, delete, and authorization scenarios

@coderabbitai

coderabbitai Bot commented Apr 22, 2026

Copy link
Copy Markdown
📝 Walkthrough

Walkthrough

Adds update and delete flows for employees and patients across DTOs, mappers, services (with authorization and uniqueness checks), REST and UI controllers, Thymeleaf edit templates, and comprehensive unit/ MVC tests.

Changes

Cohort / File(s) Summary
DTOs
src/main/java/org/example/projektarendehantering/presentation/dto/EmployeeUpdateDTO.java, src/main/java/org/example/projektarendehantering/presentation/dto/PatientUpdateDTO.java
Added update DTOs with Jakarta Bean Validation annotations and field constraints.
Mappers
src/main/java/org/example/projektarendehantering/application/service/EmployeeMapper.java, src/main/java/org/example/projektarendehantering/application/service/PatientMapper.java
Added updateEntity(dto, entity) methods that apply DTO fields to existing entities with null-guards.
Services
src/main/java/org/example/projektarendehantering/application/service/EmployeeService.java, src/main/java/org/example/projektarendehantering/application/service/PatientService.java
Added transactional update... and delete... methods; perform authorization (manager-only), existence checks, uniqueness/immutable-field enforcement, mapper updates, and persistence/deletion.
REST Controllers
src/main/java/org/example/projektarendehantering/presentation/rest/EmployeeController.java, src/main/java/org/example/projektarendehantering/presentation/rest/PatientController.java
Added PUT /{id} and DELETE /{id} endpoints wired to service update/delete methods; PatientController endpoints guarded with @PreAuthorize("hasRole('MANAGER')").
Web UI Controllers
src/main/java/org/example/projektarendehantering/presentation/web/EmployeeUiController.java, src/main/java/org/example/projektarendehantering/presentation/web/PatientUiController.java
Added manager-restricted edit (GET), update (POST with validation), and delete (POST) UI handlers; adapted constructors/imports for security adapter.
Templates
src/main/resources/templates/employees/edit.html, src/main/resources/templates/employees/list.html, src/main/resources/templates/patients/edit.html, src/main/resources/templates/patients/list.html
Added edit templates with bound forms and validation display; updated list views to include "Actions" column (edit/delete with confirmation).
Tests — Mappers
src/test/java/org/example/projektarendehantering/application/service/EmployeeMapperTest.java, src/test/java/org/example/projektarendehantering/application/service/PatientMapperTest.java
Unit tests for toDTO, toEntity, and updateEntity behaviors.
Tests — Services
src/test/java/org/example/projektarendehantering/application/service/EmployeeServiceTest.java, src/test/java/org/example/projektarendehantering/application/service/PatientServiceTest.java
Unit tests covering update/delete service logic, authorization, existence, and uniqueness/immutability checks.
Tests — Controllers / UI
src/test/java/org/example/projektarendehantering/presentation/rest/EmployeeControllerTest.java, src/test/java/org/example/projektarendehantering/presentation/rest/PatientControllerTest.java, src/test/java/org/example/projektarendehantering/presentation/web/EmployeeUiControllerTest.java, src/test/java/org/example/projektarendehantering/presentation/web/PatientUiControllerTest.java
MVC tests for REST and UI endpoints exercising update/delete flows with Spring Security and CSRF.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant Controller
    participant Service
    participant Mapper
    participant Repository

    Client->>Controller: PUT /api/{entities}/{id} + EmployeeUpdateDTO
    Controller->>Service: updateEntity(currentUser, id, dto)
    Service->>Service: require manager role (authorization)
    Service->>Repository: findById(id)
    alt entity not found
        Repository-->>Service: empty
        Service-->>Controller: throw BadRequestException(PATIENT/EMPLOYEE_NOT_FOUND)
    else entity found
        Repository-->>Service: entity
        Service->>Service: check immutable or PIN uniqueness (may throw)
        Service->>Mapper: updateEntity(dto, entity)
        Mapper-->>Service: entity (mutated)
        Service->>Repository: save(entity)
        Repository-->>Service: saved entity
        Service->>Controller: return DTO
        Controller->>Client: 200 OK + DTO
    end
Loading
sequenceDiagram
    participant Client
    participant Controller
    participant Service
    participant Repository

    Client->>Controller: DELETE /api/{entities}/{id}
    Controller->>Service: deleteEntity(currentUser, id)
    Service->>Service: require manager role (authorization)
    Service->>Repository: findById(id)
    alt not found
        Repository-->>Service: empty
        Service-->>Controller: throw BadRequestException(..._NOT_FOUND)
    else found
        Repository-->>Service: entity
        Service->>Repository: delete(entity)
        Repository-->>Service: ack
        Service-->>Controller: void
        Controller->>Client: 204 No Content
    end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested reviewers

  • OskarLundqvist33
  • mattknatt

Poem

🐰 Hop hop, the forms now dance and play,
Edit and delete come out to play,
DTOs tidy, mappers apply,
Services check and tests comply,
Templates render — managers say hooray! 🥕

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title '#75 crud for patients and users' accurately describes the main objective of implementing CRUD functionality for both patients and employees (users).
Linked Issues check ✅ Passed The PR successfully implements delete and update functionality for both patients and employees as required by issue #75, including authorization checks, validation, and both REST and UI endpoints.
Out of Scope Changes check ✅ Passed All changes directly support the CRUD objectives: DTOs, service methods, mappers, controllers (REST and UI), templates, and comprehensive tests are all aligned with delete/update requirements.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch #75-crud-for-patients-and-users

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 7

🧹 Nitpick comments (5)
src/test/java/org/example/projektarendehantering/presentation/rest/EmployeeControllerTest.java (1)

109-136: LGTM.

Update/delete endpoint tests look correct. Minor: consider also asserting the request body is actually passed to the service (e.g. ArgumentCaptor<EmployeeUpdateDTO>) to guard against accidental binding regressions.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@src/test/java/org/example/projektarendehantering/presentation/rest/EmployeeControllerTest.java`
around lines 109 - 136, Enhance updateEmployee_shouldReturnOk by capturing and
asserting the EmployeeUpdateDTO passed to employeeService.updateEmployee: create
an ArgumentCaptor<EmployeeUpdateDTO>, verify
employeeService.updateEmployee(managerActor, id, capturedDto) was called, and
assert capturedDto's fields match the input (e.g., displayName, githubUsername,
role) so the controller binding is validated alongside the existing response
assertions.
src/test/java/org/example/projektarendehantering/presentation/web/EmployeeUiControllerTest.java (2)

76-83: Test will need updating when delete is moved off GET.

Once deleteEmployee is switched to POST (see EmployeeUiController comment), this test should use post(...).with(csrf()) and assert verify(employeeService).deleteEmployee(managerActor, id) to actually exercise the service call.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@src/test/java/org/example/projektarendehantering/presentation/web/EmployeeUiControllerTest.java`
around lines 76 - 83, Update the
EmployeeUiControllerTest.deleteEmployee_shouldRedirect to match the controller
change: replace the mockMvc.get(...) call with
mockMvc.post("/ui/employees/delete/{id}", id).with(csrf()) and keep the same
assertions for 3xx redirection and redirectedUrl("/ui/employees"); additionally
assert that employeeService.deleteEmployee(managerActor, id) was invoked by
adding verify(employeeService).deleteEmployee(managerActor, id) (ensure
managerActor used in other tests or obtain it from the test setup) so the test
exercises the service call after the controller was switched from GET to POST.

21-27: Unused imports.

any and ArgumentMatchers.any are imported but not used in this file. Harmless, but worth trimming.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@src/test/java/org/example/projektarendehantering/presentation/web/EmployeeUiControllerTest.java`
around lines 21 - 27, Remove the unused static import "any"
(org.mockito.ArgumentMatchers.any) from EmployeeUiControllerTest to clean up
imports; locate the import line in the test class (EmployeeUiControllerTest) and
delete it, then re-run the build or use your IDE's "optimize imports" on the
class to ensure no other unused imports remain.
src/main/java/org/example/projektarendehantering/presentation/web/EmployeeUiController.java (1)

62-79: Prefer a dedicated not-found exception over IllegalArgumentException.

IllegalArgumentException will surface as a generic 500 unless a handler exists. EmployeeService.updateEmployee already uses BadRequestException("EMPLOYEE_NOT_FOUND", ...) for the same condition — consider mirroring that here, or simply calling the service and letting it raise, for consistent error semantics across UI and REST.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@src/main/java/org/example/projektarendehantering/presentation/web/EmployeeUiController.java`
around lines 62 - 79, The editEmployee method currently throws
IllegalArgumentException when an employee is missing; change it to use the same
domain-specific exception semantics as the service layer by throwing the
existing BadRequestException("EMPLOYEE_NOT_FOUND", ...) (or a dedicated
NotFoundException if your app has one) instead of IllegalArgumentException, or
alternatively delegate the lookup to employeeService so its existing error is
propagated; update the throwable in the lambda inside editEmployee (the call to
employeeService.getEmployee(...).orElseThrow(...)) to produce the
service-consistent exception and keep model population (EmployeeUpdateDTO,
employeeId, roles) unchanged.
src/test/java/org/example/projektarendehantering/application/service/EmployeeServiceTest.java (1)

89-117: LGTM!

Tests cover the happy paths for updateEmployee and deleteEmployee, including verifying the new mapper.updateEntity(dto, entity) interaction. Consider adding negative cases (doctor actor denied, not-found → BadRequestException, duplicate githubUsernameBadRequestException) in a follow-up to fully cover the branches in EmployeeService.updateEmployee.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@src/test/java/org/example/projektarendehantering/application/service/EmployeeServiceTest.java`
around lines 89 - 117, Add negative-unit tests for
EmployeeService.updateEmployee: create tests that call
employeeService.updateEmployee with a non-manager actor (e.g., doctorActor) and
assert that it throws the appropriate access/authorization exception and that
employeeRepository.save and employeeMapper.updateEntity are never invoked; a
test that mocks employeeRepository.findById(id) to return Optional.empty() and
asserts updateEmployee throws BadRequestException; and a test that simulates a
duplicate githubUsername scenario (mock repository/validation to indicate
existing user with same githubUsername) and assert updateEmployee throws
BadRequestException while preventing any save/update calls. Reference
methods/classes: updateEmployee, employeeService, employeeRepository.findById,
employeeRepository.save, employeeMapper.updateEntity, and the exception type
BadRequestException.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In
`@src/main/java/org/example/projektarendehantering/application/service/EmployeeService.java`:
- Around line 45-58: The updateEmployee path allows changing githubUsername
while EmployeeMapper.toEntity derives UUID from githubUsername, causing ID
collisions; modify EmployeeService.updateEmployee to reject username changes by
comparing entity.getGithubUsername() with dto.getGithubUsername() and throwing a
BadRequestException (e.g., "EMPLOYEE_USERNAME_IMMUTABLE") when they differ,
instead of attempting to find or update by a new username; this prevents
UUID/row conflicts until IDs are made independent of usernames (see
EmployeeService.updateEmployee, EmployeeMapper.toEntity, and employeeRepository
usage for locating the relevant code).

In
`@src/main/java/org/example/projektarendehantering/application/service/PatientService.java`:
- Around line 41-62: The updatePatient and deletePatient methods currently lack
authorization checks; change their signatures to accept an Actor (e.g.,
updatePatient(Actor actor, UUID id, PatientUpdateDTO dto) and
deletePatient(Actor actor, UUID id)) and call requireCanManagePatients(actor) at
the top (mirroring EmployeeService). Propagate the Actor from controllers by
injecting SecurityActorAdapter.currentUser() into PatientController endpoints
and add `@PreAuthorize` on those controller methods as defense-in-depth so only
authorized actors can call PatientService.updatePatient and
PatientService.deletePatient.
- Around line 46-51: Null-safe the personalIdentityNumber comparison in
PatientService so you never call equals on a possibly-null stored value: replace
the direct
entity.getPersonalIdentityNumber().equals(dto.getPersonalIdentityNumber()) check
with a null-safe comparison (e.g., compare dto value to entity value using
Objects.equals or check dto first) and only call
patientRepository.findByPersonalIdentityNumber(dto.getPersonalIdentityNumber())
when dto.getPersonalIdentityNumber() is non-null/non-blank; reference the
existing entity, dto, patientRepository.findByPersonalIdentityNumber(...) and
the createPatient null/blank guard when implementing the fix.

In
`@src/main/java/org/example/projektarendehantering/presentation/rest/PatientController.java`:
- Around line 32-41: Update the controller to enforce authorization and pass the
current actor to the service: add a `@PreAuthorize` annotation (e.g.
hasAnyRole('MANAGER','NURSE')) to both updatePatient and deletePatient in
PatientController, inject or reuse the existing SecurityActorAdapter to obtain
the current Actor and pass that Actor into the PatientService.updatePatient and
PatientService.deletePatient calls (update those service method signatures if
needed) so the service receives the actor for internal checks.

In
`@src/main/java/org/example/projektarendehantering/presentation/web/EmployeeUiController.java`:
- Around line 95-101: The deleteEmployee endpoint currently performs a
destructive action over GET; change the controller mapping on
deleteEmployee(UUID id) from `@GetMapping` to a state-changing mapping (prefer
`@PostMapping` or `@DeleteMapping`) so Spring’s CSRF protection applies, keep the
`@PreAuthorize`("hasRole('MANAGER')") and accept the same PathVariable signature;
then update the list.html that currently renders an <a> to instead render a
small HTML form/button that POSTs to /ui/employees/delete/{id} (include the CSRF
token and the existing JS confirm onsubmit if desired) so deletion occurs via
the protected form submission rather than a GET link.

In
`@src/main/java/org/example/projektarendehantering/presentation/web/PatientUiController.java`:
- Around line 82-86: Change the destructive deletePatient endpoint from a GET to
a POST: replace `@GetMapping`("/ui/patients/delete/{id}") with
`@PostMapping`("/ui/patients/delete/{id}") in PatientUiController and ensure the
method signature public String deletePatient(`@PathVariable` UUID id) remains the
same and still calls patientService.deletePatient(id); then update the UI
template that currently renders the delete link to submit a form POST (including
the CSRF token) instead of linking directly, so the action is protected by CSRF
and only executed via POST.

In `@src/main/resources/templates/employees/list.html`:
- Around line 34-38: The delete link issues a GET which bypasses CSRF
protection; update the employees list template to use a POST form for deletion
instead of an <a> tag: replace the delete anchor in the employees list view with
a small inline form that posts to the same URL used by
EmployeeUiController.deleteEmployee (use
th:action="@{/ui/employees/delete/{id}(id=${e.id})}" and method="post"), include
the Thymeleaf CSRF token input (th:attr or @{__csrf__} pattern your app uses)
and a submit button styled like the current "button button-small button-danger"
so behaviour is POST-based and protected by CSRF.

---

Nitpick comments:
In
`@src/main/java/org/example/projektarendehantering/presentation/web/EmployeeUiController.java`:
- Around line 62-79: The editEmployee method currently throws
IllegalArgumentException when an employee is missing; change it to use the same
domain-specific exception semantics as the service layer by throwing the
existing BadRequestException("EMPLOYEE_NOT_FOUND", ...) (or a dedicated
NotFoundException if your app has one) instead of IllegalArgumentException, or
alternatively delegate the lookup to employeeService so its existing error is
propagated; update the throwable in the lambda inside editEmployee (the call to
employeeService.getEmployee(...).orElseThrow(...)) to produce the
service-consistent exception and keep model population (EmployeeUpdateDTO,
employeeId, roles) unchanged.

In
`@src/test/java/org/example/projektarendehantering/application/service/EmployeeServiceTest.java`:
- Around line 89-117: Add negative-unit tests for
EmployeeService.updateEmployee: create tests that call
employeeService.updateEmployee with a non-manager actor (e.g., doctorActor) and
assert that it throws the appropriate access/authorization exception and that
employeeRepository.save and employeeMapper.updateEntity are never invoked; a
test that mocks employeeRepository.findById(id) to return Optional.empty() and
asserts updateEmployee throws BadRequestException; and a test that simulates a
duplicate githubUsername scenario (mock repository/validation to indicate
existing user with same githubUsername) and assert updateEmployee throws
BadRequestException while preventing any save/update calls. Reference
methods/classes: updateEmployee, employeeService, employeeRepository.findById,
employeeRepository.save, employeeMapper.updateEntity, and the exception type
BadRequestException.

In
`@src/test/java/org/example/projektarendehantering/presentation/rest/EmployeeControllerTest.java`:
- Around line 109-136: Enhance updateEmployee_shouldReturnOk by capturing and
asserting the EmployeeUpdateDTO passed to employeeService.updateEmployee: create
an ArgumentCaptor<EmployeeUpdateDTO>, verify
employeeService.updateEmployee(managerActor, id, capturedDto) was called, and
assert capturedDto's fields match the input (e.g., displayName, githubUsername,
role) so the controller binding is validated alongside the existing response
assertions.

In
`@src/test/java/org/example/projektarendehantering/presentation/web/EmployeeUiControllerTest.java`:
- Around line 76-83: Update the
EmployeeUiControllerTest.deleteEmployee_shouldRedirect to match the controller
change: replace the mockMvc.get(...) call with
mockMvc.post("/ui/employees/delete/{id}", id).with(csrf()) and keep the same
assertions for 3xx redirection and redirectedUrl("/ui/employees"); additionally
assert that employeeService.deleteEmployee(managerActor, id) was invoked by
adding verify(employeeService).deleteEmployee(managerActor, id) (ensure
managerActor used in other tests or obtain it from the test setup) so the test
exercises the service call after the controller was switched from GET to POST.
- Around line 21-27: Remove the unused static import "any"
(org.mockito.ArgumentMatchers.any) from EmployeeUiControllerTest to clean up
imports; locate the import line in the test class (EmployeeUiControllerTest) and
delete it, then re-run the build or use your IDE's "optimize imports" on the
class to ensure no other unused imports remain.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 20746ff5-ba43-4ef8-97fb-6ec2f02d857c

📥 Commits

Reviewing files that changed from the base of the PR and between 46b18db and ab51942.

📒 Files selected for processing (22)
  • src/main/java/org/example/projektarendehantering/application/service/EmployeeMapper.java
  • src/main/java/org/example/projektarendehantering/application/service/EmployeeService.java
  • src/main/java/org/example/projektarendehantering/application/service/PatientMapper.java
  • src/main/java/org/example/projektarendehantering/application/service/PatientService.java
  • src/main/java/org/example/projektarendehantering/presentation/dto/EmployeeUpdateDTO.java
  • src/main/java/org/example/projektarendehantering/presentation/dto/PatientUpdateDTO.java
  • src/main/java/org/example/projektarendehantering/presentation/rest/EmployeeController.java
  • src/main/java/org/example/projektarendehantering/presentation/rest/PatientController.java
  • src/main/java/org/example/projektarendehantering/presentation/web/EmployeeUiController.java
  • src/main/java/org/example/projektarendehantering/presentation/web/PatientUiController.java
  • src/main/resources/templates/employees/edit.html
  • src/main/resources/templates/employees/list.html
  • src/main/resources/templates/patients/edit.html
  • src/main/resources/templates/patients/list.html
  • src/test/java/org/example/projektarendehantering/application/service/EmployeeMapperTest.java
  • src/test/java/org/example/projektarendehantering/application/service/EmployeeServiceTest.java
  • src/test/java/org/example/projektarendehantering/application/service/PatientMapperTest.java
  • src/test/java/org/example/projektarendehantering/application/service/PatientServiceTest.java
  • src/test/java/org/example/projektarendehantering/presentation/rest/EmployeeControllerTest.java
  • src/test/java/org/example/projektarendehantering/presentation/rest/PatientControllerTest.java
  • src/test/java/org/example/projektarendehantering/presentation/web/EmployeeUiControllerTest.java
  • src/test/java/org/example/projektarendehantering/presentation/web/PatientUiControllerTest.java

Comment thread src/main/resources/templates/employees/list.html

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/main/java/org/example/projektarendehantering/application/service/PatientService.java (1)

30-46: ⚠️ Potential issue | 🟠 Major

Add authorization check to createPatient for consistency with update/delete.

updatePatient and deletePatient now enforce Actor authorization at the service layer via requireCanManagePatients, but createPatient has no such check. Although current controllers protect the endpoints with @PreAuthorize("hasRole('MANAGER')"), the service method should enforce the same authorization as other write operations for consistency and defense-in-depth.

Proposed fix
-    public PatientDTO createPatient(PatientCreateDTO patientDTO) {
+    public PatientDTO createPatient(Actor actor, PatientCreateDTO patientDTO) {
+        requireCanManagePatients(actor);
         PatientEntity entity = patientMapper.toEntity(patientDTO);

Update call sites in PatientController.java:31 and PatientUiController.java:53 to pass actor parameter.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@src/main/java/org/example/projektarendehantering/application/service/PatientService.java`
around lines 30 - 46, The createPatient method lacks the same service-layer
authorization as updatePatient/deletePatient; modify
PatientService.createPatient to accept an Actor parameter and call
requireCanManagePatients(actor) at the start (same pattern as updatePatient and
deletePatient) before performing the create logic, and update call sites
(PatientController and PatientUiController) to pass the actor through when
invoking createPatient so compilation and authorization are consistent.
🧹 Nitpick comments (2)
src/test/java/org/example/projektarendehantering/presentation/web/EmployeeUiControllerTest.java (1)

78-84: Add a negative CSRF assertion for delete.

This verifies the secure-delete requirement directly; the current happy-path test would still pass if CSRF protection were disabled.

Suggested test coverage
     `@Test`
     `@WithMockUser`(roles = "MANAGER")
     void deleteEmployee_shouldRedirect() throws Exception {
         UUID id = UUID.randomUUID();
         mockMvc.perform(post("/ui/employees/delete/{id}", id).with(csrf()))
                 .andExpect(status().is3xxRedirection())
                 .andExpect(redirectedUrl("/ui/employees"));
     }
+
+    `@Test`
+    `@WithMockUser`(roles = "MANAGER")
+    void deleteEmployee_withoutCsrf_shouldBeForbidden() throws Exception {
+        UUID id = UUID.randomUUID();
+
+        mockMvc.perform(post("/ui/employees/delete/{id}", id))
+                .andExpect(status().isForbidden());
+    }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@src/test/java/org/example/projektarendehantering/presentation/web/EmployeeUiControllerTest.java`
around lines 78 - 84, Add a negative CSRF test variant for the delete path: in
EmployeeUiControllerTest add a new test (or extend
deleteEmployee_shouldRedirect) that performs
mockMvc.perform(post("/ui/employees/delete/{id}", id)) without .with(csrf())
while still using `@WithMockUser`(roles = "MANAGER") and assert that the response
is rejected (e.g., status().isForbidden() / 403) to verify CSRF protection on
the delete endpoint.
src/test/java/org/example/projektarendehantering/presentation/rest/PatientControllerTest.java (1)

60-105: Add negative security coverage for the new write endpoints.

These tests prove manager success paths, but they do not fail if @PreAuthorize or CSRF protection is accidentally removed. Add at least one non-manager 403 test and one missing-CSRF rejection test for the patient mutation endpoints.

🧪 Example tests to add
+    `@Test`
+    `@WithMockUser`(roles = "NURSE")
+    void updatePatient_asNonManager_shouldBeForbidden() throws Exception {
+        UUID id = UUID.randomUUID();
+        PatientUpdateDTO input = new PatientUpdateDTO("Jane", "Doe", "19900101-1234");
+
+        mockMvc.perform(put("/api/patients/{id}", id)
+                        .with(csrf())
+                        .contentType(MediaType.APPLICATION_JSON)
+                        .content(objectMapper.writeValueAsString(input)))
+                .andExpect(status().isForbidden());
+    }
+
+    `@Test`
+    `@WithMockUser`(roles = "MANAGER")
+    void deletePatient_withoutCsrf_shouldBeForbidden() throws Exception {
+        UUID id = UUID.randomUUID();
+
+        mockMvc.perform(delete("/api/patients/{id}", id))
+                .andExpect(status().isForbidden());
+    }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@src/test/java/org/example/projektarendehantering/presentation/rest/PatientControllerTest.java`
around lines 60 - 105, Add negative security tests for the patient write
endpoints: createPatient_shouldReturnOk, updatePatient_shouldReturnOk and
deletePatient_shouldReturnNoContent. For each mutation endpoint add one test
that uses a non-manager `@WithMockUser` (e.g., role "USER") and asserts
status().isForbidden() when posting/putting/deleting the same payload/UUID, and
one test that omits .with(csrf()) and asserts the request is rejected
(status().isForbidden() or CSRF-specific rejection) to ensure `@PreAuthorize` and
CSRF protection are enforced; keep using mockMvc and the same endpoints
(/api/patients and /api/patients/{id}) and verify patientService is not invoked
in the forbidden cases (verifyNoInteractions(patientService) or
verify(patientService, never()).deletePatient(...)).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In
`@src/main/java/org/example/projektarendehantering/application/service/PatientService.java`:
- Around line 30-46: The createPatient method lacks the same service-layer
authorization as updatePatient/deletePatient; modify
PatientService.createPatient to accept an Actor parameter and call
requireCanManagePatients(actor) at the start (same pattern as updatePatient and
deletePatient) before performing the create logic, and update call sites
(PatientController and PatientUiController) to pass the actor through when
invoking createPatient so compilation and authorization are consistent.

---

Nitpick comments:
In
`@src/test/java/org/example/projektarendehantering/presentation/rest/PatientControllerTest.java`:
- Around line 60-105: Add negative security tests for the patient write
endpoints: createPatient_shouldReturnOk, updatePatient_shouldReturnOk and
deletePatient_shouldReturnNoContent. For each mutation endpoint add one test
that uses a non-manager `@WithMockUser` (e.g., role "USER") and asserts
status().isForbidden() when posting/putting/deleting the same payload/UUID, and
one test that omits .with(csrf()) and asserts the request is rejected
(status().isForbidden() or CSRF-specific rejection) to ensure `@PreAuthorize` and
CSRF protection are enforced; keep using mockMvc and the same endpoints
(/api/patients and /api/patients/{id}) and verify patientService is not invoked
in the forbidden cases (verifyNoInteractions(patientService) or
verify(patientService, never()).deletePatient(...)).

In
`@src/test/java/org/example/projektarendehantering/presentation/web/EmployeeUiControllerTest.java`:
- Around line 78-84: Add a negative CSRF test variant for the delete path: in
EmployeeUiControllerTest add a new test (or extend
deleteEmployee_shouldRedirect) that performs
mockMvc.perform(post("/ui/employees/delete/{id}", id)) without .with(csrf())
while still using `@WithMockUser`(roles = "MANAGER") and assert that the response
is rejected (e.g., status().isForbidden() / 403) to verify CSRF protection on
the delete endpoint.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 0127aa94-af9a-45fe-a496-afd2f17a0458

📥 Commits

Reviewing files that changed from the base of the PR and between ab51942 and 69f192f.

📒 Files selected for processing (13)
  • src/main/java/org/example/projektarendehantering/application/service/EmployeeService.java
  • src/main/java/org/example/projektarendehantering/application/service/PatientService.java
  • src/main/java/org/example/projektarendehantering/presentation/rest/PatientController.java
  • src/main/java/org/example/projektarendehantering/presentation/web/EmployeeUiController.java
  • src/main/java/org/example/projektarendehantering/presentation/web/PatientUiController.java
  • src/main/resources/templates/employees/edit.html
  • src/main/resources/templates/employees/list.html
  • src/main/resources/templates/patients/list.html
  • src/test/java/org/example/projektarendehantering/application/service/EmployeeServiceTest.java
  • src/test/java/org/example/projektarendehantering/application/service/PatientServiceTest.java
  • src/test/java/org/example/projektarendehantering/presentation/rest/PatientControllerTest.java
  • src/test/java/org/example/projektarendehantering/presentation/web/EmployeeUiControllerTest.java
  • src/test/java/org/example/projektarendehantering/presentation/web/PatientUiControllerTest.java
✅ Files skipped from review due to trivial changes (2)
  • src/main/resources/templates/patients/list.html
  • src/main/resources/templates/employees/edit.html
🚧 Files skipped from review as they are similar to previous changes (8)
  • src/main/resources/templates/employees/list.html
  • src/test/java/org/example/projektarendehantering/presentation/web/PatientUiControllerTest.java
  • src/main/java/org/example/projektarendehantering/application/service/EmployeeService.java
  • src/test/java/org/example/projektarendehantering/application/service/EmployeeServiceTest.java
  • src/test/java/org/example/projektarendehantering/application/service/PatientServiceTest.java
  • src/main/java/org/example/projektarendehantering/presentation/rest/PatientController.java
  • src/main/java/org/example/projektarendehantering/presentation/web/EmployeeUiController.java
  • src/main/java/org/example/projektarendehantering/presentation/web/PatientUiController.java

@LinusWestling
LinusWestling merged commit 39c4415 into main Apr 22, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

delete/update users and patients

2 participants